Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(Authoring): Show component info button when adding a component #1475

Merged
merged 8 commits into from
Nov 12, 2023
Merged

feat(Authoring): Show component info button when adding a component #1475

merged 8 commits into from
Nov 12, 2023

Conversation

geoffreykwan
Copy link
Member

Changes

Wherever you can add a new component, show an info button. The info button opens a dialog which contains a description of the component type as well as a preview.

Test

Make sure the component info works when

  • creating a new step
  • adding a component to a step that already exists

Closes #1457

@geoffreykwan geoffreykwan self-assigned this Oct 18, 2023
@geoffreykwan geoffreykwan marked this pull request as ready for review October 18, 2023 20:44
hirokiterashima
hirokiterashima previously requested changes Oct 18, 2023
View reviewed changes
Copy link
Member

@hirokiterashima hirokiterashima left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start. I think the general layout and behavior that we want are all there. Let's discuss the actual text and example(s) for each component together as a group.

I added some code change suggestions inline. Let me know what you think.

I'm also wondering if the code would be cleaner if the ComponentInfo class has description, label, and previewContent fields, and add a getInfo(componentType) function to ComponentInfoService so you query the ComponentInfo object for those fields, like this:

const info = ComponentInfoService.getInfo(componentType)
const label = info.getLabel();
const description = info.getDescription();
const previewContent = info.getPreviewContent();

Then we won't need ComponentInfoService.get[Label/Description/PreviewContent]() functions.

src/app/authoring-tool/add-component/choose-new-component/choose-new-component.component.html Outdated Show resolved Hide resolved
src/assets/wise5/authoringTool/addNode/add-your-own-node/add-your-own-node.component.html Outdated Show resolved Hide resolved
src/assets/wise5/authoringTool/components/component-selector/component-selector.component.ts Outdated Show resolved Hide resolved
src/assets/wise5/authoringTool/components/component-selector/component-selector.component.ts Outdated Show resolved Hide resolved
src/assets/wise5/authoringTool/components/component-selector/component-selector.component.ts Outdated Show resolved Hide resolved
src/assets/wise5/authoringTool/components/component-selector/component-selector.component.ts Outdated Show resolved Hide resolved
src/assets/wise5/authoringTool/components/component-selector/component-selector.component.ts Outdated Show resolved Hide resolved
src/assets/wise5/authoringTool/components/component-selector/component-selector.component.ts Outdated Show resolved Hide resolved
src/assets/wise5/authoringTool/components/component-selector/component-selector.component.ts Outdated Show resolved Hide resolved
src/assets/wise5/components/ComponentInfo.ts Outdated Show resolved Hide resolved
@hirokiterashima
Copy link
Member

Changs look good.

I'm wondering whose job it is to get ComponentInfo and Component instances for the specific componentType: is it ComponentSelectorComponent, or ComponentInfoDialog?

Right now, ComponentSelectorComponent does those things, but I wonder if it should just be passing the componentType as the sole data to the ComponentInfoDialogComponent, and let the ComponentInfoDialogComponent handle the the instantiation of those objects.

(modified)
this.dialog.open(ComponentInfoDialogComponent, {
   data: this.componentType
   panelClass: 'dialog-lg'
});

The benefit of this approach is that it prevents the ComponentSelectorComponent (and other future components that will need to launch ComponentInfoDialogComponent) from haven't to do extra computation, making it a light-weight, pass-through component (input is componentType and output is also componentType)

What do you think?

breity
breity approved these changes Nov 8, 2023
View reviewed changes
Copy link
Member

@breity breity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. 👍

I updated the component type descriptions and modified the examples to use content from our library units.

I notice there are several component types that don't work properly or don't show any content in preview mode (Discussion, Peer Chat, Show Group Work, Show My Work, Summary). We should create issues to make these show preview content that users can interact with.

@hirokiterashima hirokiterashima dismissed their stale review November 8, 2023 20:01

Add change requests have been addressed

hirokiterashima
hirokiterashima approved these changes Nov 8, 2023
View reviewed changes
Copy link
Member

@hirokiterashima hirokiterashima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

It might be good to merge develop and test locally first, just to make sure that this change set will work with Angular 16.

hirokiterashima
hirokiterashima requested changes Nov 8, 2023
View reviewed changes
Copy link
Member

@hirokiterashima hirokiterashima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we found that copying multiple components in the step view tries to make a request to

http://localhost/api/author/config/NaN 400 (Bad Request)

And this error is thrown in the js console:

ERROR Error: Uncaught (in promise): HttpErrorResponse: {"headers":{"normalizedNames":{},"lazyUpdate":null},"status":400,"statusText":"OK","url":"http://localhost/api/author/config/NaN","ok":false,"name":"HttpErrorResponse","message":"Http failure response for http://localhost/api/author/config/NaN: 400 OK","error":null}
    at resolvePromise (zone.js:1211:31)
    at resolvePromise (zone.js:1165:17)
    at zone.js:1278:17
    at _ZoneDelegate.invokeTask (zone.js:406:31)
    at core.mjs:23896:55
    at AsyncStackTaggingZoneSpec.onInvokeTask (core.mjs:23896:36)
    at _ZoneDelegate.invokeTask (zone.js:405:60)
    at Object.onInvokeTask (core.mjs:24197:33)
    at _ZoneDelegate.invokeTask (zone.js:405:60)
    at Zone.runTask (zone.js:178:47)

Can you take a look?

hirokiterashima
hirokiterashima approved these changes Nov 9, 2023
View reviewed changes
Copy link
Member

@hirokiterashima hirokiterashima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@geoffreykwan geoffreykwan merged commit a37426e into develop Nov 12, 2023
2 checks passed
@geoffreykwan geoffreykwan deleted the issue-1457-component-info branch November 12, 2023 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@breity breity breity approved these changes

@hirokiterashima hirokiterashima hirokiterashima approved these changes

Assignees

@geoffreykwan geoffreykwan

Labels
None yet
Projects
WISE Development
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat(Authoring): Show component info button when adding a component
3 participants
@geoffreykwan @hirokiterashima @breity